Skip to content

Bulk AG Overlap for bf16 on gfx950 - #713

Open
alextmagro wants to merge 2 commits into
devfrom
hipkittens_fused_bulkag
Open

Bulk AG Overlap for bf16 on gfx950#713
alextmagro wants to merge 2 commits into
devfrom
hipkittens_fused_bulkag

Conversation

@alextmagro

@alextmagro alextmagro commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Enables fused wgrad AG + dgrad GEMM for BF16 + gfx950.

Adds additional test coverage and layer tests, and makes fused/bulk_fused the default overlap scheme for gfx950, with fallback to no overlap at the moment for unaligned shapes, dtypes, and reduce-scatter.

1.282x Geomean speedup for wgrad AG + dgrad GEMM step

Comment thread tests/pytorch/distributed/test_comm_gemm_overlap.py
Comment thread tests/pytorch/distributed/test_comm_gemm_overlap.py
Comment thread tests/pytorch/distributed/test_comm_gemm_overlap.py Outdated
Comment thread tests/pytorch/distributed/run_layer_with_overlap.py
Comment thread transformer_engine/pytorch/module/base.py Outdated
Comment thread transformer_engine/pytorch/module/base.py
Comment thread transformer_engine/common/gemm/kittens/cdna4/fused_ag_gemm.cpp
@github-actions

Copy link
Copy Markdown

Reviewed the full diff of this PR against the base tip (16 files, +443/-63): the hipKittens bulk AG+GEMM kernel path (BULK template parameter, persistent_bulk_ag_bf16_gemm, run_bulk_nn), the fused_overlap_bulk_ag C++ entry point and its gemm.cpp dispatch, the Python eligibility gating in base.py, the ub_bulk_dgrad default flip in transformer.py, and the distributed test additions.

Overall the approach looks sound — the BULK specialization cleanly reuses the existing persistent GEMM body while redirecting the gather destination and compiling out the per-tile spin-wait that a bulk overlap doesn't need, and the pybind change correctly fixes reset_fused_ag_gemm_cache having been registered only under #ifndef USE_ROCM, which would have made destroy_ub() raise on ROCm.

Left 7 inline comments. The substantive ones are the new tests not actually distinguishing "fused kernel ran" from "fused kernel declined" (the reported name list reflects setup config, not the runtime gate), an unchecked shape coupling between the Userbuffers region and the dgrad GEMM in hk_bulk_ag_gemm, and a dtype-check relaxation on the non-bulk path that may be unintentional. The rest are nits.

Copyright headers: OK.

# `fused_overlap_bulk_ag` is a CommOverlapP2P entry point
opts.p2p = True
elif opts.p2p:
warnings.warn("Point-2-point comms are not supported with bulk overlap.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider updating the original warning message. Perhaps something like:

Non-fused point-2-point comms are not supported with bulk overlap

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original warning message is from upstream, so I want to leave as is. I have added the missing IS_HIP_EXTENSION guards throughout the file to make it more clear this is a difference from upstream.

@alextmagro alextmagro added the ci-level 3 CI test level 3 label Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-level 3 CI test level 3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants